Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add reproject on table, and derive crs from geometry #93

Merged
merged 1 commit into from
Nov 23, 2024

Conversation

evetion
Copy link
Owner

@evetion evetion commented Nov 23, 2024

Like the previous releases, this exported reproject still clashes on with GeometryOps, but at least operates on a table now, instead of on a column only (and forgetting to set the crs metadata on the table). A new breaking release will have to change this.

This also enables writing using the crs of the (first) geometry in the table, as requested by @alex-s-gardner.

Also drops the GeoJSON export, as pointed out by @asinghvi17.

@evetion evetion merged commit 91a6593 into master Nov 23, 2024
12 checks passed
@asinghvi17
Copy link
Contributor

hmm, is this necessary? GeometryOps already does this in GeometryOps.reproject - the capability is enabled in apply here.

https://github.com/JuliaGeo/GeometryOps.jl/blob/fc1aad94fe2803d8d36b44368d4abe2753c6e86f/GeometryOpsCore/src/apply.jl#L209-L221

That being said it only operates on the first geometry column. Maybe it should operate on all of them. But at some point I think it makes sense to centralize around GeometryOps for this, since GDAL reprojection is almost always slower than going through Proj...

Comment on lines 23 to +39
function getcrs(table)
if GeoInterface.isfeaturecollection(table)
return GeoInterface.crs(table)
elseif first(DataAPI.metadatasupport(typeof(table)))
end
crs = geomcrs(table)
if !isnothing(crs)
return crs
end
if first(DataAPI.metadatasupport(typeof(table)))
crs = DataAPI.metadata(table, "GEOINTERFACE:crs", nothing)
if isnothing(crs) # fall back to searching for "crs" as a string
crs = DataAPI.metadata(table, "crs", nothing)
end
return crs
end
nothing
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be great to upstream into GeoInterface, maybe if we make it a bit more generic to Tables.jl geometries?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, ideally as a package extension on DataAPI?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DataAPI is already a dependency i think (but maybe that PR is still open

@evetion
Copy link
Owner Author

evetion commented Nov 23, 2024

That being said it only operates on the first geometry column. Maybe it should operate on all of them. But at some point I think it makes sense to centralize around GeometryOps for this, since GDAL reprojection is almost always slower than going through Proj...

I wanted to extend GeometryOpsCore, as stated in the other issue. FWIW, I'm not sure if GO is faster than GDAL, as we need to extract the coordinates from GDAL, and GDAL itself uses PROJ (so their internal path might be faster than the Julia route?). I can imagine we specialize on the geometry type (especially once #92 is merged).

edit: Before I introduce even more type piracy, I expect to introduce a spatial vector type and a specific geodataframe type after #92.

@rafaqz
Copy link
Contributor

rafaqz commented Nov 26, 2024

I'm pretty sure GO is faster than GDAL for this, having written both functions.

First calling Proj.jl directly like GO does has so little overhead, and second converting geometries to GDAL objects as an intermediate is relatively expensive. If you have GDAL geoms already GO is probably still competitive.

But ArchGDAL doing more is fine with me anyway.

(But personally I don't contribute much here anymore, GDAL geoms have so many problems and complexity. Focussing efforts may be worthwhile given our community size)

@evetion
Copy link
Owner Author

evetion commented Nov 26, 2024

I didn't mean to cause offense 😅

GDAL will eventually be the fallback driver, anything on the Julia side will indeed be much faster with GO. And I fully understand focusing more on native stuff.

@rafaqz
Copy link
Contributor

rafaqz commented Nov 27, 2024

Not at all offended! Just thinking about allocation of effort

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants